Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

readme: Update documentation of assertion macros #237

Conversation

michael-platzer
Copy link
Contributor

During my recent changes to the assertion macros in assertions.svh as part of #233, I missed that these are documented in the repository's README file. This PR updates the README to reflect those changes.

Copy link
Collaborator

@niwis niwis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Two nitpicks for consistency

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@michael-platzer
Copy link
Contributor Author

Thanks @niwis for the super fast review. I committed your suggestions. Also, I updated the sentence highlighting the relationship with lowrisc's assertion macros, which made me realize that #233 has broken the previous compatibility with their macros. Please let me know if you consider this to be an issue that needs to be mitigated in some form.

@niwis
Copy link
Collaborator

niwis commented Oct 4, 2024

Hmm good point. Maybe we need to refine the include guards or rename the macros in the next major release. Can you open an issue for future reference?

@niwis niwis merged commit ef7d003 into pulp-platform:master Oct 4, 2024
2 checks passed
@michael-platzer michael-platzer deleted the michael-platzer/readme/update-assert-macros branch October 4, 2024 10:33
@michael-platzer
Copy link
Contributor Author

Can you open an issue for future reference?

Sure, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants